PYTHON-1982 Update Invalid Document error message to include doc#1854
PYTHON-1982 Update Invalid Document error message to include doc#1854blink1073 merged 5 commits intomongodb:masterfrom
Conversation
navjots18
changed the title
|
Thanks @navjots18! It looks like there is a segmentation fault in the C implementation. Are you able to reproduce that failure locally? I can take a closer look on Monday, but https://dnmtechs.com/handling-segmentation-fault-errors-in-python-3-programming/ is a good resource. |
|
Hi @blink1073 , it was happening because i was assigning some variables to eType and eValue and then using Py_DECREF on the variables. Changed this which fixed the segmentation error on my local |
bson/_cbsonmodule.c
Outdated
| PyErr_Fetch(&etype, &evalue, &etrace); | ||
| PyObject *InvalidDocument = _error("InvalidDocument"); | ||
|
|
||
| if (PyErr_GivenExceptionMatches(etype, InvalidDocument)) { |
There was a problem hiding this comment.
Can you please reduce the nesting by using if (InvalidDocument && PyErr_GivenExceptionMatches(etype, InvalidDocument))?
| try: | ||
| elements.append(_element_to_bson(key, value, check_keys, opts)) | ||
| except InvalidDocument as err: | ||
| raise InvalidDocument(f"Invalid document {doc} | {err}") from err |
There was a problem hiding this comment.
I'm not sure this is the pattern we want, in the C version of this code we recursively call write_dict() for subdocuments which means if we end up erroring in a nested field the error will be something like:
Invalid document {"a": {"b": {"c": ...}}} | Invalid document {"b": {"c": ...}} | Invalid document {"c": ...} | ...
Is that the intended behavior?
There was a problem hiding this comment.
@ShaneHarvey You're right, we can check the top_level param to prevent this from happening
| if (PyErr_Occurred()) { | ||
| PyObject *etype, *evalue, *etrace; | ||
| PyErr_Fetch(&etype, &evalue, &etrace); | ||
| PyObject *InvalidDocument = _error("InvalidDocument"); |
There was a problem hiding this comment.
There's a lot of missing error/NULL checks in this code. I'm not comfortable merging this for 4.9 so it will need to wait for the next release.
There was a problem hiding this comment.
@blink1073 @ShaneHarvey I took context from code written in the same file, would appreciate if can you point out some existing code from where i can see what all error/Nulls check to put here?
There was a problem hiding this comment.
Hi @navjots18, apologies for the delay. I think all we're missing in this block is initializing the etype, evalue, and etrace to NULL as we do here. Otherwise I agree this matches the rest of the existing code block.
There was a problem hiding this comment.
Hey @blink1073 fixed these issues, can you review again?
blink1073
changed the title
blink1073
changed the title
3 participants
Updated bson and cbson code to add the doc in the Invalid Document error message and added Unit test cases.
Note: Make sure to run to
python _setup.py build_ext --inplaceto install C file changes